resource/aws_lambda_function: Ignore the VPC configuration if it is empty #1341
Conversation
|
|
ETA on this merge? |
|
+1 |
radeksimko
added
the
upstream-terraform
|
Hi @joshuaspence There's an ongoing effort in the core of Terraform to improve the language so we don't need to fix such things on resource-level and can specify I'm ok with patching this locally for the time being, but there's one particular use case which is already broken and needs to be fixed first before we consider this patch. See my example below. resource "aws_vpc" "main" {
cidr_block = "10.10.0.0/16"
}
resource "aws_subnet" "test" {
cidr_block = "10.10.0.0/24"
vpc_id = "${aws_vpc.main.id}"
}
resource "aws_security_group" "test" {
vpc_id = "${aws_vpc.main.id}"
name = "radek-test"
}
resource "aws_lambda_function" "test" {
filename = "lambda.zip"
function_name = "radek-test"
role = "${aws_iam_role.iam_for_lambda.arn}"
handler = "exports.example"
runtime = "nodejs4.3"
vpc_config {
subnet_ids = ["${aws_subnet.test.id}"]
security_group_ids = ["${aws_security_group.test.id}"]
}
}
resource "aws_iam_role_policy" "iam_policy_for_lambda" {
name = "iam_policy_for_lambda_radek"
role = "${aws_iam_role.iam_for_lambda.id}"
policy = <<EOF
{
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Action": [
"logs:CreateLogGroup",
"logs:CreateLogStream",
"logs:PutLogEvents"
],
"Resource": "arn:aws:logs:*:*:*"
},
{
"Effect": "Allow",
"Action": [
"ec2:CreateNetworkInterface",
"ec2:DescribeNetworkInterfaces",
"ec2:DeleteNetworkInterface"
],
"Resource": [
"*"
]
}
]
}
EOF
}
resource "aws_iam_role" "iam_for_lambda" {
name = "iam_for_lambda_radek"
assume_role_policy = <<EOF
{
"Version": "2012-10-17",
"Statement": [
{
"Action": "sts:AssumeRole",
"Principal": {
"Service": "lambda.amazonaws.com"
},
"Effect": "Allow",
"Sid": ""
}
]
}
EOF
}now remove the VPC configuration: and try This is a valid use-case we need to support. While it wasn't caused by this PR, your PR is likely touching the same codepath and also needs to take this into consideration. I hope it makes sense. |
radeksimko
added
the
waiting-response
radeksimko
added
the
size/M
|
any updates on this? or any workaround? |
|
@radeksimko, I'm not sure why we need to consider the other bug when merging this pull request. There is an existing bug and it's impossible to remove the VPC config but - as you say - it wasn't caused by this PR. This config breaks if it is run twice: resource "aws_lambda_function" "test" {
filename = "lambda.zip"
function_name = "radek-test"
role = "${aws_iam_role.iam_for_lambda.arn}"
handler = "index.example"
runtime = "nodejs4.3"
vpc_config {
subnet_ids = []
security_group_ids = []
}
}
resource "aws_iam_role" "iam_for_lambda" {
name = "iam_for_lambda_radek"
assume_role_policy = <<EOF
{
"Version": "2012-10-17",
"Statement": [
{
"Action": "sts:AssumeRole",
"Principal": {
"Service": "lambda.amazonaws.com"
},
"Effect": "Allow",
"Sid": ""
}
]
}
EOF
}Like so: echo 'exports.example = (event, context)=>(null)' > index.js && zip lambda.zip index.js
terraform apply -auto-approve && terraform apply -auto-approve
...
aws_lambda_function.test: Modifying... (ID: radek-test)
vpc_config.#: "0" => "1"
Error: Error applying plan:
1 error(s) occurred:
* aws_lambda_function.test: 1 error(s) occurred:
* aws_lambda_function.test: vpc_config is <nil>Could you explain why this prevents us from merging this pull request? Edit: I understand now; this pull request now makes the removal failure of the VPC config silent if the lists are empty. I think I'd actually prefer that as it does at least fix the re-running problem. It doesn't seem like it's possible to remove the VPC config anyway - with the existing provider I get the following error: Removing the This is already reported in #2509 |
radeksimko
added
the
service/lambda
radeksimko
changed the title
|
We've been using this patch for our terraform deployments at work and it'd very useful. But, today we hit a case of the |
|
I've updated the branch in this PR as described in my last comment and I think I've addressed all the issues. I've added some testcases that used to trigger the panic and fixed that failure. I also made sure that the old non-VPC related tests pass (because they failed after rebasing on master). My branch is https://github.com/mdlavin/terraform-provider-aws/tree/lambda-empty_vpc_config @grahamlyons if you want to update your PR branch with my changes, then this PR will get the updates |
bflad
removed
the
waiting-response
|
Is there anything I can do to help this PR get merged? This PR is showing as having merge conflicts, by the branch that I pointed at in the comment from a month ago does not seem to be conflicting. I think that either the original PR owner @grahamlyons or a admin can update the branch on this PR |
|
@mdlavin, the original owner of this is actually @joshuaspence but I'm flattered that you thought it was me 😊 Perhaps rebasing the upstream master against your branch, @joshuaspence, will resolve the conflicts. @radeksimko, will that be sufficient to get this merged? |
|
@joshuaspence can you update your PR? |
|
Sorry about the silence here, I keep forgetting to come back to this. I'll rebase it now. |
I have a `lambda_function` module, which supports both EC2 classic and VPC. The problem I have, however, is that there is no way to specify a null configuration for `vpc_config`. This pull request changes the behavior so that the following Terraform configuration is //ignored//, instead of failing with an error (the current behavior):
```
resource "aws_lambda_function" "test" {
# ...
vpc_config {
security_group_ids = []
subnet_ids = []
}
}
```
See also #1187 and #1190.
|
The issue mentioned by @radeksimko is in PR #3473. We should be able to move forward with both. |
|
@loivis the changes in this PR address the crash as well, so if this merges then nothing else should be needed. @radeksimko a fix for this problem has been available for a long time, I've been testing with it since I submitted my branch. Is there any hope of getting it merged in so we can avoid more rebasing? |
|
Just commenting here to say that it would be much appreciated if these fixes could get merged and released in the next version as it would be great not to have to use two different modules for VPC and non-VPC lambda functions! |
|
Hi there, Thx! |
|
just stumbled into the same issue - anything preventing this from being merged @radeksimko? |
|
Maybe @bflad? |
|
sorry to be a pain, but this is a huge must have for a lot of folks, and has been ready for a very long time... ping @bflad |
|
To make merging as easy as possible, I've updated my branch to be rebased on master. The branch with fixes is https://github.com/mdlavin/terraform-provider-aws/tree/lambda-empty_vpc_config. That branch includes both the original fixes and a fix for the problem in #1341 (comment) above too |
|
I don't mind rebasing this, I just haven't bothered doing so because it seems that the upstream is not interested in this change. |
|
I've been disappointed about the lack of interest in this and other PRs in this project too. My attempt at providing a rebased version was only to make it as easy as possible for the change to be accepted. I didn't mean it as a criticism of your work. One difference between our branches is that yours does not include a fix for #1341 (comment) which was initially listed as a reason not to merge the code. Feel free to cherry pick the fix for that off my branch if you want. If you want to switch this PR to point at my branch instead, I'm happy to pick up the torch of trying to get this merged. |
|
If anybody is interested in testing out this feature in a patched v1.21.0 version, I've made some Alpine Linux x64 binaries available here: https://github.com/lifeomic/terraform-provider-aws/releases/tag/v1.21.0_patched_5f7d0def |
|
any ETA on merge? |
|
The fixes for properly handling Given that the empty list configuration for I'm reviewing this pull request right now and will likely merge this in with one panic prevention fix in a followup commit, which I'll note in my review. |
bflad
removed
the
upstream-terraform
| DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { | ||
| if v, ok := d.GetOk("vpc_config"); ok { | ||
| configs := v.([]interface{}) | ||
| config, ok := configs[0].(map[string]interface{}) |
There was a problem hiding this comment.
Thanks @joshuaspence for submitting this with an acceptance test! This was definitely on the right track.
One note here: to prevent potential panics, we should ensure the length of configs before trying to reference the first element, e.g.
if v, ok := d.GetOk("vpc_config"); ok && len(v.([]interface{})) > 0 {Style nitpick: Also, we can remove the nesting if return early:
if old != "0" && new != "1" {
return false
}
v, ok := d.GetOk("vpc_config")
if !ok || len(v.([]interface{})) == 0 {
return false
}
// other logic :)
I'll be rebasing this PR and applying a followup commit so we can get this released today, rather then making you do any additional work or having this linger further. 👍
There was a problem hiding this comment.
It turns out there are some very particular nuances when trying to work with DiffSuppressFunc in this complex manner, that was causing failures with the other existing acceptance tests. Landed on this:
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
if d.Id() == "" || old == "1" || new == "0" {
return false
}
if d.HasChange("vpc_config.0.security_group_ids") || d.HasChange("vpc_config.0.subnet_ids") {
return false
}
return true
},Which passes the new test as well as everything existing. 👍
32 tests passed (all tests)
--- PASS: TestAccAWSLambdaFunction_nilDeadLetterConfig (15.56s)
--- PASS: TestAccAWSLambdaFunction_importS3 (16.88s)
--- PASS: TestAccAWSLambdaFunction_expectFilenameAndS3Attributes (29.75s)
--- PASS: TestAccAWSLambdaFunction_versioned (34.90s)
--- PASS: TestAccAWSLambdaFunction_importLocalFile_VPC (34.93s)
--- PASS: TestAccAWSLambdaFunction_s3 (23.64s)
--- PASS: TestAccAWSLambdaFunction_importLocalFile (41.79s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_noRuntime (0.49s)
--- PASS: TestAccAWSLambdaFunction_tracingConfig (47.44s)
--- PASS: TestAccAWSLambdaFunction_DeadLetterConfigUpdated (48.33s)
--- PASS: TestAccAWSLambdaFunction_concurrency (51.13s)
--- PASS: TestAccAWSLambdaFunction_VPC (53.15s)
--- PASS: TestAccAWSLambdaFunction_versionedUpdate (54.28s)
--- PASS: TestAccAWSLambdaFunction_VPC_withInvocation (56.19s)
--- PASS: TestAccAWSLambdaFunction_DeadLetterConfig (58.60s)
--- PASS: TestAccAWSLambdaFunction_localUpdate (30.32s)
--- PASS: TestAccAWSLambdaFunction_localUpdate_nameOnly (29.58s)
--- PASS: TestAccAWSLambdaFunction_updateRuntime (69.71s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_nodeJs43 (27.51s)
--- PASS: TestAccAWSLambdaFunction_EmptyVpcConfig (54.81s)
--- PASS: TestAccAWSLambdaFunction_s3Update_unversioned (31.47s)
--- PASS: TestAccAWSLambdaFunction_s3Update_basic (37.98s)
--- PASS: TestAccAWSLambdaFunction_envVariables (82.18s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_python27 (37.28s)
--- PASS: TestAccAWSLambdaFunction_VPCUpdate (85.83s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_python36 (33.66s)
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_java8 (39.17s)
--- PASS: TestAccAWSLambdaFunction_VPCRemoval (91.22s)
--- PASS: TestAccAWSLambdaFunction_basic (91.89s)
--- PASS: TestAccAWSLambdaFunction_concurrencyCycle (94.38s)
--- PASS: TestAccAWSLambdaFunction_tags (45.34s)
--- PASS: TestAccAWSLambdaFunction_encryptedEnvVariables (99.08s)
|
This has been released in version 1.35.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
I have a
lambda_functionmodule, which supports both EC2 classic and VPC. The problem I have, however, is that there is no way to specify a null configuration forvpc_config. This pull request changes the behavior so that the following Terraform configuration is //ignored//, instead of failing with an error (the current behavior):See also #1187 and #1190.
Fixes #443